Fix some bugs in TypeCoercion rule #3407

andygrove · 2022-09-08T22:51:54Z

Which issue does this PR close?

Part of #3390

Rationale for this change

This PR fixes a number of bugs that I discovered when running a popular SQL benchmark. It does not fix all of them.

What changes are included in this PR?

Fix schema bug that affected queries with more than 2 inputs (union, intersect, etc)
Add type coercion for BETWEEN
Add workaround for a bug with + INTERVAL
Add coercion support for Date32 and Date64

Are there any user-facing changes?

No

codecov-commenter · 2022-09-09T00:41:48Z

Codecov Report

Merging #3407 (f81fb7c) into master (73447b5) will increase coverage by 0.13%.
The diff coverage is 81.96%.

@@            Coverage Diff             @@
##           master    #3407      +/-   ##
==========================================
+ Coverage   85.49%   85.63%   +0.13%     
==========================================
  Files         296      296              
  Lines       54331    54512     +181     
==========================================
+ Hits        46448    46679     +231     
+ Misses       7883     7833      -50

Impacted Files	Coverage Δ
datafusion/expr/src/binary_rule.rs	`84.59% <50.00%> (+0.08%)`	⬆️
datafusion/optimizer/src/type_coercion.rs	`90.90% <72.97%> (-8.06%)`	⬇️
datafusion/optimizer/tests/integration-test.rs	`88.88% <100.00%> (+4.88%)`	⬆️
datafusion/physical-expr/src/planner.rs	`92.68% <0.00%> (-0.87%)`	⬇️
datafusion/sql/src/planner.rs	`81.06% <0.00%> (-0.62%)`	⬇️
datafusion/optimizer/src/reduce_outer_join.rs	`98.19% <0.00%> (-0.61%)`	⬇️
datafusion/expr/src/logical_plan/plan.rs	`77.19% <0.00%> (ø)`
datafusion/proto/src/lib.rs	`93.85% <0.00%> (+0.33%)`	⬆️
datafusion/expr/src/expr_schema.rs	`63.47% <0.00%> (+0.59%)`	⬆️
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…used by recent merges to master

alamb

👍 I had a small question about casting Date32 -> date64 but otherwise this looks like a nice improvement to me

alamb · 2022-09-09T14:27:48Z

datafusion/optimizer/src/type_coercion.rs

-            )?),
-            _ => DFSchemaRef::new(DFSchema::empty()),
-        };
+        // get schema representing all available input fields. This is used for data type


alamb · 2022-09-09T14:28:41Z

datafusion/optimizer/src/type_coercion.rs

+                        DataType::Date32 | DataType::Date64 | DataType::Timestamp(_, _),
+                        &DataType::Interval(_),
+                    ) => {
+                        // Arrow `can_cast_types` says we cannot cast an Interval to


I don't think it makes sense to cast an interval to a specific point in time -- aka I think DataFusion's coerce_types is incorrect

I am tracking this as part of #3419

alamb · 2022-09-09T14:29:11Z

datafusion/optimizer/src/type_coercion.rs

+                let expr_type = expr.get_type(&self.schema)?;
+                let low_type = low.get_type(&self.schema)?;
+                let coerced_type = comparison_coercion(&expr_type, &low_type)
+                    .ok_or_else(|| DataFusionError::Internal("".to_string()))?;


Why the empty error message? That seems like it may be confusing

oops .. forgot to go back and finish this. I have pushed a fix.

alamb · 2022-09-09T14:29:24Z

datafusion/optimizer/src/type_coercion.rs

@@ -244,6 +283,34 @@ mod test {
        Ok(())
    }

+    #[test]
+    fn binary_op_date32_add_interval() -> Result<()> {
+        //CAST(Utf8("1998-03-18") AS Date32) + IntervalDayTime("386547056640")


datafusion/expr/src/binary_rule.rs

liukun4515 · 2022-09-09T14:37:50Z

I will review tomorrow, please hold it

alamb · 2022-09-12T11:07:37Z

I will review tomorrow, please hold it

@liukun4515 do you still want us to hold this PR for your review?

andygrove · 2022-09-12T16:20:52Z

I'd prefer that we get these bug fixes merged to unblock some other items. Arrow is still following the "Commit-Then-Review" (C-T-R) policy:

A policy governing code changes which permits developers to make changes at will, with the possibility of being retroactively [vetoed](http://www.apache.org/foundation/glossary.html#Veto)

In other words, we can continue with the review after merge, and revert the PR if it proves to be problematic.

alamb · 2022-09-12T17:03:17Z

I agree -- let's merge this PR and we can either revert it or address comments in a follow on PR if/when provided by @liukun4515

Thanks all!

ursabot · 2022-09-12T17:13:01Z

Benchmark runs are scheduled for baseline = 3e56a0f and contender = 7427a80. 7427a80 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Fix schema bug in TypeCoercion rule

318b601

github-actions bot added the optimizer Optimizer rules label Sep 8, 2022

Add type coercion for between

d450346

andygrove changed the title ~~Fix schema bug in TypeCoercion rule~~ Improve TypeCoercion rule Sep 8, 2022

andygrove marked this pull request as draft September 8, 2022 23:39

andygrove added 2 commits September 8, 2022 17:39

add workaround for INTERVAL

f4283f5

fix regression

4e12037

github-actions bot added the core Core DataFusion crate label Sep 9, 2022

andygrove requested review from liukun4515 and alamb September 9, 2022 00:46

andygrove marked this pull request as ready for review September 9, 2022 00:46

andygrove added the bug Something isn't working label Sep 9, 2022

upmerge

045c2bc

github-actions bot removed the core Core DataFusion crate label Sep 9, 2022

add support for coercion between Date32/Date64 and fix regressions ca…

73ebe71

…used by recent merges to master

github-actions bot added the logical-expr Logical plan and expressions label Sep 9, 2022

andygrove changed the title ~~Improve TypeCoercion rule~~ Fix some bugs in TypeCoercion rule Sep 9, 2022

alamb approved these changes Sep 9, 2022

View reviewed changes

andygrove mentioned this pull request Sep 9, 2022

Fix inconsistencies in type coercion logic #3419

Open

fix error message

e1a0ab6

alamb approved these changes Sep 9, 2022

View reviewed changes

update comments and link to follow-on issue

f81fb7c

andygrove mentioned this pull request Sep 9, 2022

DataFusion 12.0.0 Release #3097

Closed

4 tasks

alamb merged commit 7427a80 into apache:master Sep 12, 2022

andygrove deleted the fix-type-coercion-bugs branch September 12, 2022 17:08

This was referenced Nov 14, 2022

between with NULL literals does not work: can't be evaluated because there isn't a common type to coerce the types to #1193

Closed

Support type cast in union #2125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some bugs in TypeCoercion rule #3407

Fix some bugs in TypeCoercion rule #3407

andygrove commented Sep 8, 2022 •

edited

Loading

codecov-commenter commented Sep 9, 2022 •

edited

Loading

alamb left a comment

alamb Sep 9, 2022

alamb Sep 9, 2022

andygrove Sep 9, 2022

alamb Sep 9, 2022

andygrove Sep 9, 2022

alamb Sep 9, 2022

liukun4515 commented Sep 9, 2022

alamb commented Sep 12, 2022

andygrove commented Sep 12, 2022

alamb commented Sep 12, 2022

ursabot commented Sep 12, 2022

Fix some bugs in TypeCoercion rule #3407

Fix some bugs in TypeCoercion rule #3407

Conversation

andygrove commented Sep 8, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented Sep 9, 2022 • edited Loading

Codecov Report

alamb left a comment

Choose a reason for hiding this comment

alamb Sep 9, 2022

Choose a reason for hiding this comment

alamb Sep 9, 2022

Choose a reason for hiding this comment

andygrove Sep 9, 2022

Choose a reason for hiding this comment

alamb Sep 9, 2022

Choose a reason for hiding this comment

andygrove Sep 9, 2022

Choose a reason for hiding this comment

alamb Sep 9, 2022

Choose a reason for hiding this comment

liukun4515 commented Sep 9, 2022

alamb commented Sep 12, 2022

andygrove commented Sep 12, 2022

alamb commented Sep 12, 2022

ursabot commented Sep 12, 2022

andygrove commented Sep 8, 2022 •

edited

Loading

codecov-commenter commented Sep 9, 2022 •

edited

Loading